Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pipeline-based script loading and retries #1499

Merged
merged 4 commits into from
Feb 20, 2022
Merged

Pipeline-based script loading and retries #1499

merged 4 commits into from
Feb 20, 2022

Conversation

marcbachmann
Copy link
Collaborator

@marcbachmann marcbachmann commented Feb 1, 2022 •
edited
Loading

This PR builds on #1497 (first three comments)
The last two commits are relevant for the new script load behavior.

Changelog

  • 🚀 Scripts are loaded into the pipeline instead in advance
    • It improves performance during first load
    • Improves cluster support with scripts
  • 🐛 Automatically retry failed evalsha commands if the script is missing in redis, this improves the resiliency
  • 🐛 Use the same script caching logic for standalone execution of scripts
  • 🐛 Only send the scripts to a socket exactly once

@marcbachmann marcbachmann changed the title Pipelined evalsha Pipeline-based script loading and retries Feb 1, 2022
@marcbachmann marcbachmann mentioned this pull request Feb 1, 2022
Merged
@marcbachmann marcbachmann mentioned this pull request Feb 1, 2022
Closed
marcbachmann
marcbachmann commented Feb 1, 2022
View reviewed changes
lib/script.ts Outdated

// Standalone mode (regular)
function usingStandalone(script, redis, args, options, callback) {
if (redis._addedScriptHashes[script.sha]) {
Copy link
Collaborator Author

@marcbachmann marcbachmann Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing those decisions in here, we could move it into the command.toWritable function.
But we don't have any stream context there. https://github.com/luin/ioredis/blob/b8177479c348aa4bbd467fa944d61fe9b35aec19/lib/redis/index.ts#L790

If we would pass in the stream to the toWritable function, we could maintain the script cache per cluster node or socket instead on the redis instance.

@marcbachmann
Copy link
Collaborator Author

marcbachmann commented Feb 3, 2022 •
edited
Loading

@luin what do you think about those changes to make the scripts more reliable?
The script implementation is now much more straight forward.
Just how a socket-like object like the pipeline gets passed to the redis instance could be better at the moment. Because of that the check is a bit more complicated during write.

But in general it completely eliminates all caching issues as we just load the script once with every new connection. Previously it resulted in quite a lot of NOSCRIPT errors. I think this drawback is good enough for most cases.
We could also only use eval when multi was used. To guarantee that the script is always there in that case. For the other cases it will do the fallback.

marcbachmann
marcbachmann commented Feb 5, 2022
View reviewed changes
lib/script.ts

// Resend the same custom evalsha command that gets transformed to an eval
// in case it's not loaded yet on the connectionDo an eval as fallback, redis will hash and load it
const resend = new this.Command(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove this fallback if order of delivery needs to be guaranteed.
It will still clear the cache for the next command that gets sent to redis.
https://github.com/luin/ioredis/pull/1499/files#diff-3618aaced6e025d978d2cdde354e756687916385e9024868d0ad3fff887c55c6R24

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking to include this breaking change in the next major version #1504 so we can just retry custom commands in pipeline as well.

marcbachmann
marcbachmann commented Feb 5, 2022
View reviewed changes
lib/redis/index.ts Outdated

if (stream) {
if (stream.isPipeline)
stream.write(command.toWritable(stream.destination.redis.stream));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the pipeline we could move the buffer logic into the Redis class itself.
Maybe even persist the pipeline itself in the queue instead of calling sendCommand that often.

@marcbachmann marcbachmann mentioned this pull request Feb 6, 2022
Closed
@luin luin mentioned this pull request Feb 6, 2022
Closed
8 tasks
@luin
Copy link
Collaborator

luin commented Feb 12, 2022

Hey @marcbachmann 👋,

Sorry for the delay. I'm planning v5 in #1504. So do you mind change the target branch of this PR to v5 and resolve the conflicts?

@marcbachmann marcbachmann changed the base branch from master to v5 February 12, 2022 15:56
@marcbachmann
Copy link
Collaborator Author

@luin I've rebased this.
At least one cluster.disconnect test is missing as I've removed the script based ones:
https://github.com/luin/ioredis/pull/1499/files#diff-25757dbbc9670af28c244c85ae7b36c1224b1963223299f4df48963785141a6fL415

@luin luin merged commit 332980f into redis:v5 Feb 20, 2022
@luin
Copy link
Collaborator

luin commented Feb 20, 2022

Awesome! Nice and clean improvements!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luin luin luin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marcbachmann @luin